Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Polish metrics page and flow #2461

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Polish metrics page and flow #2461

wants to merge 25 commits into from
+384 −209

Conversation

rebecarubio
Copy link
Contributor

@rebecarubio rebecarubio commented Jan 9, 2025 •
edited
Loading

Description

This PR implements a new design for "outcomes" page, now renamed as "report" page. It polishes the creation, listing, deletion and edition of metrics for the canvas assignment.

Screenshots

image

Changes

  • Renames Outcomes tab for Report
  • Implements new design to choose the "Reporting level" now called "Data collection"
  • Adds new icons to differentiate Choise Metrics from Scale Metrics
  • Places the questions toolbar on the bottom of the page
  • Changes Edit and Delete buttons for icons
  • Removes Checkbox in edit mode
  • Adds a Modal to open in edit mode
  • Adds a confirmation Modal before deleting a metric

Notes to reviewer

None

Related issues

None

@richardolsson
Copy link
Member

I can't find the new UI. Was the tab lost in the merge?

https://app-zetkin-wi1wgb361-zetkin.vercel.app/organize/1/projects/9/areaassignments/6762b039174d1f80e2824b1a

@rebecarubio
Copy link
Contributor Author

I can't find the new UI. Was the tab lost in the merge?

https://app-zetkin-wi1wgb361-zetkin.vercel.app/organize/1/projects/9/areaassignments/6762b039174d1f80e2824b1a

The tab is not lost but the file is called "logging" so I didn't want to do the full renaming of the tab before talking with a native speaker (as we discuss in K&K), so now in the branch we see the "report" file that is the old one.

@rebecarubio rebecarubio changed the title Polish metrics page and flow WIP Polish metrics page and flow Jan 15, 2025
@rebecarubio rebecarubio changed the title WIP Polish metrics page and flow Polish metrics page and flow Jan 16, 2025
richardolsson
richardolsson requested changes Jan 19, 2025
View reviewed changes
Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks really nice now, and the code looks mostly very good as well. But the text needs to be internationalized and there are some minor code style issues that I've commented on below.

src/features/areaAssignments/components/MetricCard.tsx Outdated Show resolved Hide resolved
src/pages/organize/[orgId]/projects/[campId]/areaassignments/[areaAssId]/report.tsx Outdated Show resolved Hide resolved
src/pages/organize/[orgId]/projects/[campId]/areaassignments/[areaAssId]/report.tsx Outdated Show resolved Hide resolved
src/pages/organize/[orgId]/projects/[campId]/areaassignments/[areaAssId]/report.tsx Outdated Show resolved Hide resolved
src/pages/organize/[orgId]/projects/[campId]/areaassignments/[areaAssId]/report.tsx Outdated Show resolved Hide resolved
ziggabyte
ziggabyte requested changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@ziggabyte ziggabyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions that arose from playing around with the deployed version!

src/features/areaAssignments/l10n/messageIds.ts Outdated Show resolved Hide resolved
src/pages/organize/[orgId]/projects/[campId]/areaassignments/[areaAssId]/report.tsx Outdated Show resolved Hide resolved
src/pages/organize/[orgId]/projects/[campId]/areaassignments/[areaAssId]/report.tsx Outdated
justifyContent="space-between"
width="100%"

{metricBeingDeleted?.definesDone && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional, that we get to this same dialog but without a list of selectable questions, when we attempt to delete the only yes/no-question (which ofcourse then is the one that defines "done")? It felt like it could potentially be a mistake, but I'm not sure.
bild

Copy link
Contributor Author

@rebecarubio rebecarubio Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I'm leaning towards removing the option to delete the only yes/no question if there are no other questions to choose from. Alternatively, we could display a message like: 'To delete {question}, you must first create a new choice question that defines if the mission was successful.' What do you think @ziggabyte ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think i have good judgement around this type of UX flow 😂 I don't know honestly, I think it should be asked to the designers and/or richard because I think they have an idea of how this should flow that i don't remember...

src/pages/organize/[orgId]/projects/[campId]/areaassignments/[areaAssId]/report.tsx Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ziggabyte ziggabyte ziggabyte requested changes

@richardolsson richardolsson Awaiting requested review from richardolsson

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rebecarubio @richardolsson @ziggabyte